HDDS-15839. Implement SCM container ID export manager - #10813
HDDS-15839. Implement SCM container ID export manager#10813sarvekshayr wants to merge 11 commits into
Conversation
devmadhuu
left a comment
devmadhuu
left a comment
There was a problem hiding this comment.
Thanks @sarvekshayr for the patch. Largely the PR looks good, just few minor comments.
There was a problem hiding this comment.
@sarvekshayr , thanks for splitting this out. Some general comments
- Refactor out all the File/Directory related code to a new class ExportFileManager
- Describe directory layout in ExportFileManager (very important since changing it is an incompatible change)
- Except for ContainerExportManager, use "Export" instead of "ContainerExport" as the prefix for the classnames
See also the comments inlined.
@sarvekshayr , indeed, this PR is also quite big. Let's work on ExportFileManager and directory layout in a separated JIRA. |
Created HDDS-15935. |
devmadhuu
left a comment
devmadhuu
left a comment
There was a problem hiding this comment.
Thanks @sarvekshayr for improving the patch. Largely LGTM +1. Just a nit for rare case. Pls check.
devmadhuu
left a comment
devmadhuu
left a comment
There was a problem hiding this comment.
Thanks @sarvekshayr for improving the patch. LGTM +1
@sarvekshayr , are we going to do HDDS-15935 first and then this? Is it the case? |
Not in this PR. HDDS-15935 will land in a follow-up PR after this one is merged. |
ArafatKhan2198
left a comment
ArafatKhan2198
left a comment
There was a problem hiding this comment.
Thanks for the patch @sarvekshayr mostly looks good!
Some comments you can consider.
Please let me know if I am missing out on something.
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
Not in this PR. HDDS-15935 will land in a follow-up PR after this one is merged.
It is fine only if this PR does not create any files.
Otherwise, we will need the disk layout description (i.e. HDDS-15935) first.
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
@sarvekshayr , Thanks for the update! Please see some quick comments inlined. BTW, the role of ContainerExportManager and ExportFileManager should be:
- ContainerExportManager: manage jobs
- ExportFileManager: manage files
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
@sarvekshayr , thanks for the update! Please see the comments inlined.
BTW, before submitting a PR, please review it yourself first. If there are unused methods/code, remove them.
…nd Remove unused code
There was a problem hiding this comment.
@sarvekshayr , thanks for the update!
This change is still too big. I tried to review this multiple times but failed due to the size of the PR. (People usually cannot spend a whole day reviewing a single PR. They have other things to do. Recall the point of "Rapid Feedback" in the Google AI Overview on Multiple Small Changes.)
Please split it into two PRs.
- Do not mix simple changes with complicated changes. In this case, the metrics can be added later. Tried to limit the PR size to < 30KB. The current PR size is 57KB.
% git diff apache/master...sarvekshayr/export-tool-4 > a.patch
% ls -lh a.patch
-rw-r--r--@ 1 szetszwo staff 57K Aug 13 20:48 a.patch
- The test is only 15KB. It seems not enough compared to the code size (~40KB). Try using AI to generate tests.
% ls -lh hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/export/TestContainerExportManager.java
-rw-r--r--@ 1 szetszwo staff 15K Aug 13 20:51 hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/export/TestContainerExportManager.java
- Do not add short methods which are used only once.
See also the comments inlined and also https://issues.apache.org/jira/secure/attachment/13083920/10813_review.patch. Sorry that I only reviewed a part of the PR due to time limit.
|
Thanks @szetszwo for the review! I agree that this PR became quite large since it pulls in all the functions for
If this plan looks good, I’ll start refactoring and update this PR accordingly. |
Sure, the plan looks great! A rule of thumb is that simple change PR (renaming, adding conf/metrics, simple refactoring, etc.) can be larger while a complicated PR (logic changes, new code, non-trivial bug fixes, etc.) should be as small as possible. We want to make a complicated problem obvious and easy to be understood. |
4 participants
What changes were proposed in this pull request?
Adds the SCM leader-side container ID export worker. Jobs paginate through
ContainerManager.getContainerIDs()with lifecycle/health filters, write sharded text files, and pack them into a TAR.ContainerExportLimits.java- Shared page/shard defaults and capsContainerExportStatus.java- Job status snapshot (State, filters, row count, TAR path, error)ContainerExportManager.java- Async export worker: submit, paginate, shard, TAR append, job trackingContainerExportMetrics.java- Export job counters and last-success gaugesTestContainerExportManager.java- Unit tests with mocked ContainerManagerPart 4 in splitting HDDS-15496 #10673 container ID export.
What is the link to the Apache JIRA
HDDS-15839
How was this patch tested?
Added a new unit test class
TestContainerExportManager(no RPC or CLI yet).CI: https://github.com/sarvekshayr/ozone/actions/runs/29718653670